Skip to content

Tidying up clock config of H5. #2695

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Mar 31, 2025
Merged

Tidying up clock config of H5. #2695

merged 1 commit into from
Mar 31, 2025

Conversation

dojyorin
Copy link
Contributor

Summary

The generic clock config for STM32H5 series will be made as common as possible.
As a result, only differences will be the number of peripherals that vary by part number, clock source and peripherals used by each variant, etc.

We have also tidied up the writing style, such as commenting out.

This fixes H503KBU which was not included in #2598 fix.

Common changes

  • Common to all
    • USB uses HSI48
  • Common in H503
    • Peripherals: RCC_PERIPHCLK_ADCDAC RCC_PERIPHCLK_LPUART1 RCC_PERIPHCLK_USB RCC_PERIPHCLK_SPI1 RCC_PERIPHCLK_SPI2 RCC_PERIPHCLK_SPI3
  • Common in H562 H563 H573
    • SDMMC1 operates at 50MHz.
    • Peripherals: RCC_PERIPHCLK_SDMMC1 RCC_PERIPHCLK_ADCDAC RCC_PERIPHCLK_LPUART1 RCC_PERIPHCLK_USB RCC_PERIPHCLK_SPI1 RCC_PERIPHCLK_SPI2 RCC_PERIPHCLK_SPI3 RCC_PERIPHCLK_SPI4 RCC_PERIPHCLK_SPI5 RCC_PERIPHCLK_SPI6
      • RCC_PERIPHCLK_SPI4 and RCC_PERIPHCLK_SPI5 is only for models with high pin count.

Variant-specific changes

  • H503CB(T-U)
    • Writing style has been improved.
  • H503KBU
  • H503RBT
    • Writing style has been improved.
  • variant: NUCLEO_H503RB
    • Follow H503RBT.
  • H562R(G-I)T
    • Writing style has been improved.
    • PLL2R and USB value has been made consistent with H563R(G-I)T, H563IIKxQ_H573IIKxQ and H563Z(G-I)T values.
  • variant: WEACT_H562RG
    • Follow H562R(G-I)T.
  • H563IIKxQ_H573IIKxQ
    • Writing style has been improved.
    • OSPI removed.
  • variant: STM32H573I_DK
    • Follow H563IIKxQ_H573IIKxQ.
    • Since SAI is enabled, I have not touched PLL2 multiplex.
  • H563R(G-I)T
    • Writing style has been improved.
    • PLL2R and SDMMC1 value has been made consistent with H562R(G-I)T, H563IIKxQ_H573IIKxQ and H563Z(G-I)T values.
  • H563Z(G-I)T
    • Writing style has been improved.
    • SDMMC1 value has been made consistent with H562R(G-I)T, H563IIKxQ_H573IIKxQ and H563R(G-I)T values.
  • variant: NUCLEO_H563ZI
    • Follow H563Z(G-I)T.
    • Since PLL is HSE, adjust multiplex so that final frequency is same as H563Z(G-I)T.

@dojyorin
Copy link
Contributor Author

Ah, I'm stupid.
I forgot to add HSI48 and CSI to the clock sources for NUCLEO_H563ZI.
Sorry.

@fpistm
Copy link
Member

fpistm commented Mar 19, 2025

Hi @dojyorin
I don't see any benefits from this PR except for the H503K.

@dojyorin
Copy link
Contributor Author

@fpistm
I've been working on a project that uses H5 continuously for some time, and I thought that if I could tidy up the code base, I'd be able to comfortably expand future H5 variants.
If this PR increases the likelihood of serious issues, I think you should withdraw it, but if it's OK from the perspective of tidying up the code base, would you consider merging it?

@dojyorin
Copy link
Contributor Author

For example, by organizing things in this PR, it will be easier to visually distinguish the differences in a diff editor, making it easier to add new variants and apply patches.

I'm currently developing several custom H5 boards, and when I was referencing the code of existing variants to add a variant, I was a little confused by the subtle variations in numbers and notation.

I submitted a PR to reduce experiences like this.

@dojyorin dojyorin changed the title Unify clock config of H5. Tidying up clock config of H5. Mar 19, 2025
@dojyorin
Copy link
Contributor Author

@fpistm
Thank you for understanding the purpose.
And thank you for correcting my work.

I'll fix the corrections this weekend.

fpistm
fpistm requested changes Mar 25, 2025
View reviewed changes
Copy link
Member

@fpistm fpistm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I understand your point to change config order will help ton compare anyway changing several values just to have less difference is risky and probably add regressions.
Current config is know to work and I have no time to test this PR on each target to ensure it is correct.

Please revert to CSI the Nucleo H503RB config as the one you set is not correct.

@dojyorin
Copy link
Contributor Author

dojyorin commented Mar 27, 2025 •
edited
Loading

I thought I had checked datasheets, but it seems I still didn't fully understand it.
I'm sorry for bothering you repeatedly.
Thank you for your detailed explanation.
I will fix it soon.

@fpistm fpistm added the enhancement New feature or request label Mar 28, 2025
@fpistm fpistm added this to the 2.11.0 milestone Mar 28, 2025
@fpistm fpistm force-pushed the chore-unify-h5-clock branch from b393c0c to 7968394 Compare March 28, 2025 14:12
@fpistm
Copy link
Member

fpistm commented Mar 28, 2025

I've updated the PR. Honestly, anytime a new H5 board will be added the clock config will not be aligned with your reordering.
Cube MX generate config in different order and ofeten with different values.
I have some doubt about the disco but as said would not like spent too much time on this kind of PR.
For me it is ok like this.

@fpistm fpistm self-requested a review March 28, 2025 14:15
fpistm
fpistm approved these changes Mar 28, 2025
View reviewed changes
Copy link
Member

@fpistm fpistm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. No test performed.

@github-project-automation github-project-automation bot moved this from In progress to Reviewer approved in STM32 core based on ST HAL Mar 28, 2025
@dojyorin
Copy link
Contributor Author

dojyorin commented Mar 28, 2025 •
edited
Loading

I understood that code cleanup does not make much sense.
As this project is largely founded on your contribution, I have decided to follow your opinion.

@fpistm
Copy link
Member

fpistm commented Mar 28, 2025

Not agreed. I understand your point about diff but you changed some configurations for specific hardware which could bring some regressions. And as stated above cube mx does not follow this rules. Anyway any contributions are welcome. I'd like to have more contributions and hope to find new maintainers. Moreover some of your changes seems go to have a better configurations. 😉

@fpistm fpistm force-pushed the chore-unify-h5-clock branch from 7968394 to 444d2e2 Compare March 31, 2025 09:12
@fpistm fpistm merged commit 06547a2 into stm32duino:main Mar 31, 2025
24 checks passed
@github-project-automation github-project-automation bot moved this from Reviewer approved to Done in STM32 core based on ST HAL Mar 31, 2025
@dojyorin dojyorin deleted the chore-unify-h5-clock branch March 31, 2025 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fpistm fpistm fpistm approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
STM32 core based on ST HAL
Status: Done
Milestone
2.11.0
Development

Successfully merging this pull request may close these issues.
2 participants
@dojyorin @fpistm